Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zstd compression support #1371

Closed
wants to merge 11 commits into from
Closed

Conversation

martinabeleda
Copy link
Contributor

Motivation

See #1236

Solution

Add zstd as a CompressionEncoding and as a feature/optional dependency.

New to rust so looking for some feedback on my approach to testing.

Copy link
Contributor

@h33p h33p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a hyper dev, but approach seems sensible and extensible. I would look into the function visibility comment, though.

tonic/src/codec/compression.rs Outdated Show resolved Hide resolved
@mpetri
Copy link

mpetri commented Jun 17, 2023

@martinabeleda any updates on this CR?

Are you waiting for feedback from the actual maintainers? @LucioFranco would you be willing to accept this change? I think this will lead to substantial performance improvement for especially compressible messages.

@martinabeleda
Copy link
Contributor Author

@martinabeleda any updates on this CR?

Are you waiting for feedback from the actual maintainers? @LucioFranco would you be willing to accept this change? I think this will lead to substantial performance improvement for especially compressible messages.

@mpetri I just have a few of your comments to address but other than that I'm waiting for maintainers. I'll have some time to finish the PR later today so can try to reach @LucioFranco on discord

@LucioFranco
Copy link
Member

Hi sorry for the delay. I think the comments make sense but I am taking a small break for the next few weeks so the progress has been slow here.

Copy link

@mpetri mpetri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good now. One minor comment on the default value of the compressor

tonic/src/codec/compression.rs Outdated Show resolved Hide resolved
@QuentinPerez
Copy link
Contributor

QuentinPerez commented Aug 6, 2023

Hi @LucioFranco,

I have tested it in production, and it works perfectly! Thank you, @martinabeleda! When do you think we can merge it? I don't want to stay on a fork with that patch forever.

@LucioFranco
Copy link
Member

@martinabeleda seems like there are some failures around feature flags in CI.

@QuentinPerez
Copy link
Contributor

@martinabeleda do you have the time to fix the CI issue ? Otherwise I will re-open the PR with your code and the fixes for the CI.

@martinabeleda
Copy link
Contributor Author

@martinabeleda do you have the time to fix the CI issue ? Otherwise I will re-open the PR with your code and the fixes for the CI.

@QuentinPerez I haven't had time to work on the fixes so would appreciate if you could help fix CI. Thanks!!

@tottoto
Copy link
Collaborator

tottoto commented Jun 1, 2024

Resolved in #1532. Thank you!

@tottoto tottoto closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants